Skip to content

Conversation

@Joeavaikath
Copy link
Contributor

@Joeavaikath Joeavaikath commented Feb 12, 2026

Why the changes were made

  • Fixes bug: --details flag value not being respected for nonadmin
  • Enhances verb-noun order for non-admin commands
  • Fixes -o yaml output parsing for nonadmin resources

How to test the changes made

oc oadp na backup describe <backup-name> should now respect --details

Summary by CodeRabbit

  • New Features

    • Describe command now conditionally shows extended details (including resource lists) via a --details flag.
  • Improvements

    • More reliable verb→noun delegation with direct invocation for accurate flag propagation and execution.
    • Project-specific output formatting added to support custom CRD types and JSON/YAML encoding.
  • Tests

    • Expanded coverage for delegation, flag propagation, and output encoding/formatting.
  • Chores

    • .gitignore updated to ignore vendor/.

Signed-off-by: Joseph <jvaikath@redhat.com>
@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

📝 Walkthrough

Walkthrough

Changes refactor command execution in the non-admin CLI to use fresh command instances and direct RunE invocation instead of command cloning. Updates include conditional Resource List display in backup describe, dynamic command usage strings in BSL commands, and expanded test coverage for verb-noun delegation patterns.

Changes

Cohort / File(s) Summary
Backup Details Gate
cmd/non-admin/backup/describe.go
Added showDetails boolean parameter to printNonAdminBackupDetails to conditionally fetch and display Resource List based on the --details flag.
BSL Command Construction
cmd/non-admin/bsl/bsl.go, cmd/non-admin/bsl/create.go
Updated NewCreateCommand signature to accept a dynamic use string parameter; bsl.go now passes "create" when constructing the subcommand, enabling dynamic usage strings instead of fixed command names.
Verb Execution Refactoring
cmd/non-admin/verbs/builder.go
Replaced command cloning pattern with fresh resource command instances and direct RunE/Run invocation after manual flag parsing. Removed createCommandInstance helper function. Updated delegation to retrieve resourceCmd and subCmd fresh, parse flags directly on subCmd, and invoke RunE/Run without relying on Cobra's Execute() method.
Verb Builder Test Overhaul
cmd/non-admin/verbs/builder_test.go
Replaced static field comparison tests with runtime behavior tests covering verb-noun delegation patterns, flag propagation, error scenarios (nil handler, unknown resource type, missing resource type), and multiple registered resources validation.

Sequence Diagram(s)

sequenceDiagram
    participant Client
    participant VerbBuilder
    participant ResourceCmd
    participant Subcommand
    participant FlagParser
    participant RunE

    Client->>VerbBuilder: Execute verb command
    VerbBuilder->>ResourceCmd: Get fresh resource command instance
    VerbBuilder->>Subcommand: Get noun subcommand from resource
    VerbBuilder->>Subcommand: Set remaining args
    VerbBuilder->>FlagParser: Parse flags on subcommand
    VerbBuilder->>Subcommand: Validate Args if defined
    VerbBuilder->>RunE: Invoke subcommand RunE/Run directly
    RunE->>Client: Return result
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Poem

🐰 With verbs and nouns in fresh command,
No cloning tricks across the land,
Flags flow true, details on demand,
Our CLI now executes as planned! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 2
❌ Failed checks (1 warning, 1 inconclusive)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive PR description lacks detail on testing procedures and provides minimal explanation of changes across multiple files. Expand testing instructions with specific commands, expected outputs, and verification steps for all three fixes mentioned (--details flag, verb-noun order, -o yaml output).
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'verb-noun order bugfixes' directly relates to the main change in the changeset, which involves reorganizing verb-noun command execution patterns and fixing bugs in the non-admin command structure.
Merge Conflict Detection ✅ Passed ✅ No merge conflicts detected when merging into oadp-dev

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder_test.go`:
- Around line 408-410: The test currently only logs when err.Error() does not
contain "command not found" which means it never fails on unexpected error
messages; update the assertion in the test (in builder_test.go around the
strings.Contains(err.Error(), "command not found") check) to fail the test when
the error message is not what we expect by replacing the log with a test failure
(e.g., call t.Fatalf or t.Errorf) that includes the actual err.Error() so the
test fails and reports the unexpected message.
🧹 Nitpick comments (5)
cmd/non-admin/backup/describe.go (1)

325-344: Redundant context creation inside printNonAdminBackupDetails.

A new context with timeout is created at lines 327-328, but the caller at line 38 already creates a context with the same timeout. Consider passing the existing context as a parameter instead of creating a new one. This would:

  1. Avoid redundant timeout tracking
  2. Ensure proper cancellation propagation from parent context
  3. Follow the convention of accepting context as first parameter
♻️ Suggested approach

Pass ctx from the caller into printNonAdminBackupDetails instead of creating a new context:

-func printNonAdminBackupDetails(cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration, showDetails bool) {
+func printNonAdminBackupDetails(ctx context.Context, cmd *cobra.Command, nab *nacv1alpha1.NonAdminBackup, kbClient kbclient.Client, backupName string, userNamespace string, timeout time.Duration, showDetails bool) {

Then in the showDetails block, use the passed context directly:

 		if showDetails {
-			ctx, cancel := context.WithTimeout(context.Background(), timeout)
-			defer cancel()
-
 			resourceList, err := shared.ProcessDownloadRequest(ctx, kbClient, shared.DownloadRequestOptions{

Update the call site at line 75:

-			printNonAdminBackupDetails(cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout, details)
+			printNonAdminBackupDetails(ctx, cmd, &nab, kbClient, backupName, userNamespace, effectiveTimeout, details)
cmd/non-admin/verbs/builder.go (1)

65-84: Consider using strings.Join and sorting for deterministic output.

The current implementation uses manual string concatenation and doesn't sort the resource types. While acceptable for error messages, this could produce inconsistent output order across runs.

♻️ Suggested improvement
+import (
+	"sort"
+	"strings"
+	...
+)

 func (vb *NonAdminVerbBuilder) getAvailableResourceTypes() string {
 	if len(vb.resourceRegistry) == 0 {
 		return "none"
 	}

 	var types []string
 	for resourceType := range vb.resourceRegistry {
 		types = append(types, resourceType)
 	}
+	sort.Strings(types)

-	result := ""
-	for i, t := range types {
-		if i > 0 {
-			result += ", "
-		}
-		result += t
-	}
-	return result
+	return strings.Join(types, ", ")
 }
cmd/non-admin/verbs/builder_test.go (2)

451-455: Consider asserting the duration value instead of just logging.

The test verifies the flag was received but doesn't assert the normalized value. If the duration normalization changes or breaks, the test would still pass while only logging the discrepancy.

Proposed fix for stricter validation
 	if !flagReceived {
 		t.Error("Expected duration flag --ttl to be passed to delegated command")
-	} else if receivedValue != "720h0m0s" {
-		t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue)
+	} else if receivedValue != "720h0m0s" {
+		t.Errorf("Expected duration value '720h0m0s', got: %s", receivedValue)
+	} else {
+		t.Logf("Duration flag passed with expected value: %s", receivedValue)
 	}

498-505: Map contents not validated against expected values.

The test verifies the flag was received but doesn't assert that receivedMap contains the expected entries (region=us-east-1 and bucket=my-bucket). This reduces the test's ability to catch regressions in map flag parsing.

Proposed fix to validate map contents
 	if !flagReceived {
 		t.Error("Expected map flag --config to be passed to delegated command")
 	} else {
-		t.Logf("Map flag passed successfully with %d entries", len(receivedMap))
-		if len(receivedMap) > 0 {
-			t.Logf("Map contents: %v", receivedMap)
+		if len(receivedMap) != 2 {
+			t.Errorf("Expected 2 map entries, got %d: %v", len(receivedMap), receivedMap)
+		}
+		if receivedMap["region"] != "us-east-1" {
+			t.Errorf("Expected region=us-east-1, got region=%s", receivedMap["region"])
+		}
+		if receivedMap["bucket"] != "my-bucket" {
+			t.Errorf("Expected bucket=my-bucket, got bucket=%s", receivedMap["bucket"])
 		}
+		t.Logf("Map flag passed with expected contents: %v", receivedMap)
 	}
cmd/non-admin/README.md (1)

220-245: Add language specifier to fenced code blocks.

The ASCII flow diagrams should have a language specifier (e.g., text or plaintext) to satisfy markdown linting rules.

Proposed fix
-```
+```text
 User: kubectl oadp nonadmin backup create my-backup --storage-location aws
 
 Flow:
 1. NonadminCommand
 2. → BackupCommand
 3. → CreateCommand
 4. → Flags parsed directly on CreateCommand
 5. → CreateCommand.RunE() executes

```diff
-```
+```text
 User: kubectl oadp nonadmin create backup my-backup --storage-location aws
 
 Flow:
 1. NonadminCommand
 2. → CreateCommand (verb)
 3. → Extract resource type: "backup"
 4. → Create fresh backup command instance
 5. → Get "create" subcommand from backup
 6. → Convert verb flags to args
 7. → Parse args on fresh subcommand
 8. → Execute subcommand.RunE()
</details>

</blockquote></details>

</blockquote></details>

<!-- This is an auto-generated comment by CodeRabbit for review status -->

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder.go`:
- Around line 168-172: The boolean branch only appends the flag when flagValue
== "true", so explicit false values (e.g. --details=false) are lost; update the
case "bool" in builder.go to always propagate the user's explicit value: if
flagValue == "true" append "--"+flag.Name (preserving shorthand form) otherwise
append "--"+flag.Name+"="+flagValue so that false (or any explicit boolean) is
passed through via remainingArgs.
- Around line 173-181: The stringArray/stringSlice branch currently appends the
entire bracketed String() output as one value causing a nested array on
re-parse; change the logic in the case "stringArray", "stringSlice" to detect
and handle bracket-enclosed values: if flagValue starts with "[" and ends with
"]", trim the brackets, split the inner string on commas, trim whitespace from
each item, and for each non-empty item append "--"+flag.Name and the item to
remainingArgs (so repeated flags are passed); otherwise keep the existing
single-value append behavior for non-bracket values. Ensure you continue to
check flagValue != "" && flagValue != "[]".
🧹 Nitpick comments (1)
cmd/non-admin/verbs/builder.go (1)

65-84: Consider using strings.Join and sorting for consistency.

The manual string concatenation can be simplified with strings.Join. Additionally, map iteration order in Go is non-deterministic, so error messages may show resource types in different orders across executions. Sorting provides consistent output.

♻️ Suggested refactor
+import (
+	"sort"
+	"strings"
+	...
+)

 func (vb *NonAdminVerbBuilder) getAvailableResourceTypes() string {
 	if len(vb.resourceRegistry) == 0 {
 		return "none"
 	}

 	var types []string
 	for resourceType := range vb.resourceRegistry {
 		types = append(types, resourceType)
 	}
-
-	result := ""
-	for i, t := range types {
-		if i > 0 {
-			result += ", "
-		}
-		result += t
-	}
-	return result
+	sort.Strings(types)
+	return strings.Join(types, ", ")
 }

Comment on lines 173 to 181
case "stringArray", "stringSlice":
// Handle string array/slice flags
remainingArgs = append(remainingArgs, "--"+flag.Name, flagValue)
// For array/slice flags, we need to preserve multiple values
// The String() representation might be comma-separated or bracket-enclosed
// For pflag arrays, each value should be passed with its own flag instance
// However, since we're re-parsing, the single string representation should work
// as pflag will handle splitting appropriately
if flagValue != "" && flagValue != "[]" {
remainingArgs = append(remainingArgs, "--"+flag.Name, flagValue)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate the file and examine the actual code
find . -name "builder.go" -path "*/non-admin/verbs/*" | head -5

Repository: migtools/oadp-cli

Length of output: 93


🏁 Script executed:

# Check if the test file exists
find . -name "builder_test.go" -path "*/non-admin/verbs/*" | head -5

Repository: migtools/oadp-cli

Length of output: 98


🏁 Script executed:

# List the repository structure to understand the project
ls -la

Repository: migtools/oadp-cli

Length of output: 1272


🏁 Script executed:

# Examine the builder.go file around lines 173-181
cat -n cmd/non-admin/verbs/builder.go | sed -n '165,190p'

Repository: migtools/oadp-cli

Length of output: 1408


🏁 Script executed:

# Check the broader context to understand the addFlagsToArgs function
cat -n cmd/non-admin/verbs/builder.go | sed -n '140,200p'

Repository: migtools/oadp-cli

Length of output: 2881


🏁 Script executed:

# Look at the test file to see current test coverage
cat -n cmd/non-admin/verbs/builder_test.go | sed -n '70,185p'

Repository: migtools/oadp-cli

Length of output: 4188


🏁 Script executed:

# Create a test to verify how pflag actually parses stringArray with bracket-enclosed values
cd /tmp && cat > test_pflag_parse.go << 'EOF'
package main

import (
	"fmt"
	"strings"
	
	"github.com/spf13/pflag"
)

func main() {
	tests := []struct {
		name   string
		args   []string
		desc   string
	}{
		{
			name:   "bracket-enclosed",
			args:   []string{"--items", "[a,b,c]"},
			desc:   "pflag with bracket-enclosed string",
		},
		{
			name:   "unbracketed-comma",
			args:   []string{"--items", "a,b,c"},
			desc:   "pflag with unbracketed comma-separated",
		},
		{
			name:   "repeated-flags",
			args:   []string{"--items", "a", "--items", "b", "--items", "c"},
			desc:   "pflag with repeated flags",
		},
	}

	for _, test := range tests {
		fmt.Printf("\n=== Test: %s (%s) ===\n", test.name, test.desc)
		fmt.Printf("Input args: %v\n", test.args)
		
		fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
		var items []string
		fs.StringArrayVar(&items, "items", nil, "test items")
		
		err := fs.Parse(test.args)
		if err != nil {
			fmt.Printf("Parse error: %v\n", err)
			continue
		}
		
		fmt.Printf("Parsed array: %v\n", items)
		fmt.Printf("Array length: %d\n", len(items))
		
		// Simulate what String() returns and re-parse
		if len(items) > 0 {
			// Manually construct what String() would return
			stringRepr := "[" + strings.Join(items, ",") + "]"
			fmt.Printf("\nSimulating re-parse with String() output: %q\n", stringRepr)
			
			fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
			var items2 []string
			fs2.StringArrayVar(&items2, "items", nil, "test items")
			
			err2 := fs2.Parse([]string{"--items", stringRepr})
			if err2 != nil {
				fmt.Printf("Re-parse error: %v\n", err2)
			} else {
				fmt.Printf("Re-parsed array: %v\n", items2)
				fmt.Printf("Re-parsed length: %d\n", len(items2))
				if len(items2) == 1 && items2[0] == stringRepr {
					fmt.Printf("WARNING: Re-parse treated entire bracket string as single element!\n")
				}
			}
		}
	}
}
EOF

go mod init test 2>/dev/null
go mod edit -require=github.com/spf13/pflag@latest 2>/dev/null
go mod tidy 2>&1 | head -5
go run test_pflag_parse.go

Repository: migtools/oadp-cli

Length of output: 454


🏁 Script executed:

# Use the repo's go.mod to run our test
cat > cmd/non-admin/verbs/test_pflag_parse.go << 'EOF'
package verbs

import (
	"fmt"
	"strings"
	"testing"

	"github.com/spf13/pflag"
)

func TestPflagStringArrayParsing(t *testing.T) {
	tests := []struct {
		name   string
		args   []string
		desc   string
	}{
		{
			name:   "bracket-enclosed",
			args:   []string{"--items", "[a,b,c]"},
			desc:   "pflag with bracket-enclosed string",
		},
		{
			name:   "unbracketed-comma",
			args:   []string{"--items", "a,b,c"},
			desc:   "pflag with unbracketed comma-separated",
		},
		{
			name:   "repeated-flags",
			args:   []string{"--items", "a", "--items", "b", "--items", "c"},
			desc:   "pflag with repeated flags",
		},
	}

	for _, test := range tests {
		fmt.Printf("\n=== Test: %s (%s) ===\n", test.name, test.desc)
		fmt.Printf("Input args: %v\n", test.args)
		
		fs := pflag.NewFlagSet("test", pflag.ContinueOnError)
		var items []string
		fs.StringArrayVar(&items, "items", nil, "test items")
		
		err := fs.Parse(test.args)
		if err != nil {
			fmt.Printf("Parse error: %v\n", err)
			continue
		}
		
		fmt.Printf("Parsed array: %v\n", items)
		fmt.Printf("Array length: %d\n", len(items))
		
		// Simulate what String() returns and re-parse
		if len(items) > 0 {
			// Manually construct what String() would return
			stringRepr := "[" + strings.Join(items, ",") + "]"
			fmt.Printf("\nSimulating re-parse with String() output: %q\n", stringRepr)
			
			fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
			var items2 []string
			fs2.StringArrayVar(&items2, "items", nil, "test items")
			
			err2 := fs2.Parse([]string{"--items", stringRepr})
			if err2 != nil {
				fmt.Printf("Re-parse error: %v\n", err2)
			} else {
				fmt.Printf("Re-parsed array: %v\n", items2)
				fmt.Printf("Re-parsed length: %d\n", len(items2))
				if len(items2) == 1 && items2[0] == stringRepr {
					fmt.Printf("WARNING: Re-parse treated entire bracket string as single element!\n")
				}
			}
		}
	}
}
EOF

# Now run the test from the repository root
cd . && go test -v -run TestPflagStringArrayParsing ./cmd/non-admin/verbs/

Repository: migtools/oadp-cli

Length of output: 5221


🏁 Script executed:

# Also let's check what the actual String() output looks like for stringArray flags in this codebase
grep -n "StringArray\|stringArray" cmd/non-admin/verbs/*.go | head -20

Repository: migtools/oadp-cli

Length of output: 789


🏁 Script executed:

# Let me create a simpler standalone test that can run directly
cat > /tmp/test_array_parse.go << 'EOF'
package main

import (
	"fmt"
	"strings"
	"github.com/spf13/pflag"
)

func main() {
	fmt.Println("Testing pflag stringArray parsing behavior:")
	fmt.Println("===========================================")
	
	// Test 1: What does String() return for a parsed stringArray?
	fmt.Println("\n[Test 1] Initial parse and String() output:")
	fs1 := pflag.NewFlagSet("test1", pflag.ContinueOnError)
	var items1 []string
	fs1.StringArrayVar(&items1, "items", nil, "test items")
	
	fs1.Parse([]string{"--items", "a", "--items", "b", "--items", "c"})
	fmt.Printf("  Parsed with repeated flags: %v\n", items1)
	
	// Get the flag and call String() on it
	flag1 := fs1.Lookup("items")
	stringRepr := flag1.Value.String()
	fmt.Printf("  String() output: %q\n", stringRepr)
	
	// Test 2: Re-parse the bracket-enclosed output
	fmt.Println("\n[Test 2] Re-parsing the String() output (bracket-enclosed):")
	fs2 := pflag.NewFlagSet("test2", pflag.ContinueOnError)
	var items2 []string
	fs2.StringArrayVar(&items2, "items", nil, "test items")
	
	err := fs2.Parse([]string{"--items", stringRepr})
	if err != nil {
		fmt.Printf("  Parse error: %v\n", err)
	} else {
		fmt.Printf("  Parsed result: %v\n", items2)
		fmt.Printf("  Array length: %d\n", len(items2))
		if len(items2) == 1 && strings.Contains(items2[0], "[") {
			fmt.Printf("  ❌ PROBLEM: Entire bracket string treated as single element!\n")
		} else if len(items2) == 3 {
			fmt.Printf("  ✓ OK: Array elements preserved correctly\n")
		}
	}
	
	// Test 3: What about unbracketed comma-separated?
	fmt.Println("\n[Test 3] Parsing unbracketed comma-separated string:")
	fs3 := pflag.NewFlagSet("test3", pflag.ContinueOnError)
	var items3 []string
	fs3.StringArrayVar(&items3, "items", nil, "test items")
	
	err = fs3.Parse([]string{"--items", "a,b,c"})
	if err != nil {
		fmt.Printf("  Parse error: %v\n", err)
	} else {
		fmt.Printf("  Parsed result: %v\n", items3)
		fmt.Printf("  Array length: %d\n", len(items3))
	}
}
EOF

cd . && go run /tmp/test_array_parse.go

Repository: migtools/oadp-cli

Length of output: 525


🏁 Script executed:

# Let me also verify what the actual test in builder_test.go checks
cat -n cmd/non-admin/verbs/builder_test.go | sed -n '115,125p'

Repository: migtools/oadp-cli

Length of output: 437


Fix stringArray round-trip parsing: remove bracket notation before passing to subcommand.

The String() representation of stringArray flags returns bracket-enclosed format ([a,b,c]), but when this is passed directly to the fresh subcommand's parser as --flag [a,b,c], pflag treats the entire bracket string as a single array element, resulting in [[a,b,c]] instead of [a,b,c]. The array elements are lost.

Verification result
[Test 1] Initial parse:        [a b c]
[Test 2] String() output:      "[a,b,c]"
[Test 3] Re-parse String():    [[a,b,c]]  ← entire bracket string becomes 1 element

Either strip the brackets before passing to the parser, or reconstruct using repeated flags (--flag a --flag b --flag c). The existing test only verifies flag.Changed and flag.Value.String() exist, but does not validate that array elements are correctly parsed by the delegated command.

🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder.go` around lines 173 - 181, The
stringArray/stringSlice branch currently appends the entire bracketed String()
output as one value causing a nested array on re-parse; change the logic in the
case "stringArray", "stringSlice" to detect and handle bracket-enclosed values:
if flagValue starts with "[" and ends with "]", trim the brackets, split the
inner string on commas, trim whitespace from each item, and for each non-empty
item append "--"+flag.Name and the item to remainingArgs (so repeated flags are
passed); otherwise keep the existing single-value append behavior for
non-bracket values. Ensure you continue to check flagValue != "" && flagValue !=
"[]".

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
cmd/non-admin/verbs/builder_test.go (1)

267-275: ⚠️ Potential issue | 🟡 Minor

Avoid nil deref in error assertion.

If err is nil, the test continues and calls err.Error(), which will panic. Prefer t.Fatal to stop the test immediately.

Suggested fix
 	err := verbCmd.Execute()
 	if err == nil {
-		t.Error("Expected error for unknown resource type, got nil")
+		t.Fatal("Expected error for unknown resource type, got nil")
 	}
🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder_test.go`:
- Around line 500-507: The test currently only checks flagReceived and
len(receivedMap); update it to assert the exact expected map contents by
defining an expected map (e.g., expectedMap) and comparing it to receivedMap
using a deep-equality check (reflect.DeepEqual or cmp.Diff) and failing the test
with a clear message if they differ; keep flagReceived check but replace the
loose len(receivedMap) branch with an equality assertion that verifies both keys
and values match, and include the mismatched contents in the error message so
failures show the expected vs actual maps (referencing flagReceived and
receivedMap in builder_test.go).
- Around line 453-457: The test currently only logs when the received duration
is not normalized, so change that log to a real assertion: replace the t.Logf in
the builder_test.go check that inspects flagReceived and receivedValue with
t.Fatalf or t.Errorf (e.g., t.Fatalf("Duration flag passed with value %q;
expected normalized form %q", receivedValue, "720h0m0s")) so the test fails on
regressions; keep the existing condition and variables (flagReceived,
receivedValue) and the expected string "720h0m0s".

Comment on lines 453 to 457
if !flagReceived {
t.Error("Expected duration flag --ttl to be passed to delegated command")
} else if receivedValue != "720h0m0s" {
t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Make duration normalization a real assertion.

The test logs when the value is unexpected but doesn’t fail, so regressions won’t be caught.

Suggested fix
-	if !flagReceived {
-		t.Error("Expected duration flag --ttl to be passed to delegated command")
-	} else if receivedValue != "720h0m0s" {
-		t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue)
-	}
+	if !flagReceived {
+		t.Fatal("Expected duration flag --ttl to be passed to delegated command")
+	}
+	if receivedValue != "720h0m0s" {
+		t.Errorf("Expected normalized duration value %q, got %q", "720h0m0s", receivedValue)
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if !flagReceived {
t.Error("Expected duration flag --ttl to be passed to delegated command")
} else if receivedValue != "720h0m0s" {
t.Logf("Duration flag passed with value: %s (expected normalized form)", receivedValue)
}
if !flagReceived {
t.Fatal("Expected duration flag --ttl to be passed to delegated command")
}
if receivedValue != "720h0m0s" {
t.Errorf("Expected normalized duration value %q, got %q", "720h0m0s", receivedValue)
}
🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder_test.go` around lines 453 - 457, The test
currently only logs when the received duration is not normalized, so change that
log to a real assertion: replace the t.Logf in the builder_test.go check that
inspects flagReceived and receivedValue with t.Fatalf or t.Errorf (e.g.,
t.Fatalf("Duration flag passed with value %q; expected normalized form %q",
receivedValue, "720h0m0s")) so the test fails on regressions; keep the existing
condition and variables (flagReceived, receivedValue) and the expected string
"720h0m0s".

Comment on lines 500 to 507
if !flagReceived {
t.Error("Expected map flag --config to be passed to delegated command")
} else {
t.Logf("Map flag passed successfully with %d entries", len(receivedMap))
if len(receivedMap) > 0 {
t.Logf("Map contents: %v", receivedMap)
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Assert expected map contents, not just presence.

The test currently passes even if only one entry is parsed or values are wrong.

Suggested fix
 	if !flagReceived {
-		t.Error("Expected map flag --config to be passed to delegated command")
-	} else {
-		t.Logf("Map flag passed successfully with %d entries", len(receivedMap))
-		if len(receivedMap) > 0 {
-			t.Logf("Map contents: %v", receivedMap)
-		}
-	}
+		t.Fatal("Expected map flag --config to be passed to delegated command")
+	}
+	if len(receivedMap) != 2 || receivedMap["region"] != "us-east-1" || receivedMap["bucket"] != "my-bucket" {
+		t.Errorf("Expected map entries {region: us-east-1, bucket: my-bucket}, got: %v", receivedMap)
+	}
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if !flagReceived {
t.Error("Expected map flag --config to be passed to delegated command")
} else {
t.Logf("Map flag passed successfully with %d entries", len(receivedMap))
if len(receivedMap) > 0 {
t.Logf("Map contents: %v", receivedMap)
}
}
if !flagReceived {
t.Fatal("Expected map flag --config to be passed to delegated command")
}
if len(receivedMap) != 2 || receivedMap["region"] != "us-east-1" || receivedMap["bucket"] != "my-bucket" {
t.Errorf("Expected map entries {region: us-east-1, bucket: my-bucket}, got: %v", receivedMap)
}
🤖 Prompt for AI Agents
In `@cmd/non-admin/verbs/builder_test.go` around lines 500 - 507, The test
currently only checks flagReceived and len(receivedMap); update it to assert the
exact expected map contents by defining an expected map (e.g., expectedMap) and
comparing it to receivedMap using a deep-equality check (reflect.DeepEqual or
cmp.Diff) and failing the test with a clear message if they differ; keep
flagReceived check but replace the loose len(receivedMap) branch with an
equality assertion that verifies both keys and values match, and include the
mismatched contents in the error message so failures show the expected vs actual
maps (referencing flagReceived and receivedMap in builder_test.go).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@cmd/non-admin/verbs/builder.go`:
- Around line 140-153: The current flow in builder.go parses flags and then
directly calls subCmd.RunE, which bypasses any argument validators defined via
subCmd.Args and skips important checks (e.g., backup/delete.go). Before invoking
RunE, call the Args validator by executing subCmd.Args()(or
subCmd.Args()(parsedArgs) depending on signature) with parsedArgs and
handle/return any error; only if that succeeds proceed to call
subCmd.RunE(subCmd, parsedArgs). Ensure you reference the variables and
functions used here: subCmd, remainingArgs, parsedArgs, subCmd.ParseFlags,
subCmd.Flags().Args(), subCmd.Args(), and subCmd.RunE.
🧹 Nitpick comments (1)
cmd/non-admin/verbs/builder.go (1)

65-84: Optional: sort resource types for deterministic error messages.
Map iteration order is random, so sorting avoids noisy diffs and user‑facing variability.

♻️ Suggested tweak
 import (
 	"fmt"
+	"sort"
 	"strings"

@@
-	var types []string
-	for resourceType := range vb.resourceRegistry {
-		types = append(types, resourceType)
-	}
-
-	var builder strings.Builder
-	for i, t := range types {
-		if i > 0 {
-			builder.WriteString(", ")
-		}
-		builder.WriteString(t)
-	}
-	return builder.String()
+	types := make([]string, 0, len(vb.resourceRegistry))
+	for resourceType := range vb.resourceRegistry {
+		types = append(types, resourceType)
+	}
+	sort.Strings(types)
+	return strings.Join(types, ", ")

@Joeavaikath Joeavaikath marked this pull request as draft February 12, 2026 18:37
Signed-off-by: Joseph <jvaikath@redhat.com>
@Joeavaikath Joeavaikath marked this pull request as ready for review February 12, 2026 19:37
@Joeavaikath
Copy link
Contributor Author

Joeavaikath commented Feb 12, 2026

@coderabbitai help

@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.

CodeRabbit commands

These commands are invoked using PR/Issue comments.

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai evaluate custom pre-merge check --instructions <custom-checks-instructions> --name <custom-checks-title> [--mode <error|warning>] to validate the custom pre-merge checks instructions. Defaults to error when --mode is omitted.
    • @coderabbitai ignore pre-merge checks to override pre-merge checks and get an approval on PR.
    • @coderabbitai run pre-merge checks to run pre-merge checks on the pull request.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai generate unit tests to generate unit tests for this PR.
  • @coderabbitai resolve to resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai generate configuration to create a PR that adds the current resolved configuration as .coderabbit.yaml (or show it if already present).
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore or @coderabbit ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit configuration file (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • You can also validate your configuration using the online YAML validator.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Status, support, documentation and community

  • Visit our status page to check the current availability of CodeRabbit.
  • Create a ticket on our support page for assistance with any issues or questions.
  • Visit our documentation site for detailed information on how to use CodeRabbit.
  • Join our Discord community to connect with other users and get help from the community.
  • Follow us on X/Twitter for updates and announcements.

@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

Tip

For best results, initiate chat on the files or code changes.

@Joeavaikath Hello! 👋 I'm here to help. How can I assist you with PR #118?

I can help you with:

  • Reviewing the changes in this PR
  • Explaining specific code modifications
  • Running tests or analyzing the codebase
  • Answering questions about the verb-noun order bugfixes
  • Testing the --details flag behavior you mentioned
  • Anything else related to this pull request

What would you like me to do?

@Joeavaikath
Copy link
Contributor Author

Joeavaikath commented Feb 12, 2026

@coderabbitai review

@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

1 similar comment
@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Velero's output.PrintWithFormat uses an upstream scheme that doesn't include
NonAdmin CRD types, causing "no kind is registered" errors when using -o yaml
or -o json flags. Implemented custom output package with NonAdmin scheme
registration to enable proper serialization of downstream types.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
@Joeavaikath
Copy link
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

✅ Actions performed

Full review triggered.

Check error return values from cmd.Flags().Set and io.Copy
to satisfy errcheck linter.

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Signed-off-by: Joseph <jvaikath@redhat.com>
@Joeavaikath
Copy link
Contributor Author

@coderabbitai resume

@coderabbitai
Copy link

coderabbitai bot commented Feb 12, 2026

✅ Actions performed

Reviews resumed.

@Joeavaikath Joeavaikath linked an issue Feb 13, 2026 that may be closed by this pull request
@weshayutin
Copy link
Contributor

@stillalearner please have a look

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

nonadmin APIs are not supported with get -o yaml

2 participants